Update/i18n utils unreverted #47492

lsl · 2020-11-17T09:08:07Z

Follows up on @p-jackson's #47446 to introduce useLocale, incorporates some changes from #47358 to decouple i18n-utils from getLocaleSlug (i18n-calypso package), but instead of replacing getLocaleSlug with react-i18n's useI18n, it uses its own useLocale.

i18n-utils is now completely free of all deps but does expect the LocaleProvider to be configured.

warning: Pretty sure I haven't got this set up right yet. Manual testing was showing localizeUrl wasn't working in the /new/domains/es gutenboarding context.

Please suggest anything you see that can be dropped from this PR without causing any knock on effects - e.g. we can probably drop changes to domain-picker.

Changes proposed in this Pull Request

Testing instructions

Fixes #

matticbot · 2020-11-17T09:08:09Z

Test live: https://calypso.live/?branch=update/i18n-utils-unreverted

client/components/calypso-i18n-provider/index.tsx

lsl · 2020-11-17T09:12:14Z

packages/react-i18n/src/index.tsx

 	return {
 		__: i18n.__.bind( i18n ),
 		_n: i18n._n.bind( i18n ),
 		_nx: i18n._nx.bind( i18n ),
 		_x: i18n._x.bind( i18n ),
 		isRTL: i18n.isRTL.bind( i18n ),
-		i18nLocale,


Probably need to roll this change back and keep it around for a bit - breaking change. (and widely used in calypso) ... What's -alpha mean in the version number? Is that basically like saying this isn't semver?

What's -alpha mean in the version number? Is that basically like saying this isn't semver?

That's how pre-release versions are denoted according to SemVer's specs. Ref: https://semver.org/#spec-item-9

So safe to break backward compat here then, thanks for the link 👍

I'm also not sure that @automattic/react-i18n is used anywhere outside of a8c either so I was just planning to udpate all the uses. It looks like there is a package on the npm registry but it's private.

matticbot · 2020-11-17T09:26:02Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~593 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-main                  +674 B  (+0.0%)      +99 B  (+0.0%)
entry-login                 +664 B  (+0.1%)     +113 B  (+0.0%)
entry-domains-landing       +664 B  (+0.1%)     +181 B  (+0.1%)
entry-gutenboarding         +596 B  (+0.0%)     +200 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Async-loaded Components (~52 bytes added 📈 [gzipped])

name                                           parsed_size           gzip_size
async-load-calypso-blocks-editor-launch-modal        -17 B  (-0.0%)      +52 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

p-jackson

Looking good. I still need to make some changes to my PR since it currently breaks the build I believe, which means that some of these changes will probably disappear after a rebase.

client/components/calypso-i18n-provider/index.tsx

p-jackson · 2020-11-18T07:57:43Z

packages/react-i18n/src/index.tsx

 	return {
 		__: i18n.__.bind( i18n ),
 		_n: i18n._n.bind( i18n ),
 		_nx: i18n._nx.bind( i18n ),
 		_x: i18n._x.bind( i18n ),
 		isRTL: i18n.isRTL.bind( i18n ),
-		i18nLocale,


I'm also not sure that @automattic/react-i18n is used anywhere outside of a8c either so I was just planning to udpate all the uses. It looks like there is a package on the npm registry but it's private.

p-jackson · 2020-11-18T07:57:49Z

packages/i18n-utils/src/localize-url.ts

@@ -132,3 +128,14 @@ export function localizeUrl( fullUrl: string, toLocale?: Locale ): string {
 	// Nothing needed to be changed, just return it unmodified.
 	return fullUrl;
 }
+
+export function useLocalizeUrl(): ( fullUrl: string, locale?: Locale ) => string {


Co-authored-by: Philip Jackson <p-jackson@live.com>

yuliyan

I left a few small remarks, but overall it LGTM 🚀

packages/i18n-utils/src/localize-url.ts

yuliyan · 2020-11-19T14:59:24Z

client/components/calypso-i18n-provider/index.tsx

@@ -25,7 +27,11 @@ const CalypsoI18nProvider: React.FunctionComponent = ( { children } ) => {
 		};
 	}, [] );

-	return <I18nProvider localeData={ localeData }>{ children }</I18nProvider>;
+	return (
+		<LocaleProvider localeSlug={ localeSlug || 'en' }>


Let's use config( 'i18n_default_locale_slug' ) for fallback instead of 'en' string literal.

Great, TIL! There's some other places I can use this config value too 👍

Exposed and used the hard coded one in i18n-utils, I think we need to strip these i18n config values from calypso usage and rely on the utils package unless we want to configure LocaleProvider with the configuration as well?

This is done in #47613

lsl · 2020-11-20T06:50:25Z

#47612
#47613

p-jackson and others added 9 commits November 16, 2020 18:18

Add locale to i18n-utils so it can be used and set in React components

bb931b2

remove calypso

5bcd83d

fix tests ~

d051237

wrap the new localizeUrl function

4069986

Use LocaleProvider in calypso

6752212

more test fixes

3832b59

Missed export for localizeUrl underlying

1ab6219

Pivot to useLocalizeUrl from useI18nUtils

d9056d2

Don't lie about types

b5a7725

lsl commented Nov 17, 2020

View reviewed changes

client/components/calypso-i18n-provider/index.tsx Show resolved Hide resolved

lsl commented Nov 17, 2020

View reviewed changes

lsl mentioned this pull request Nov 17, 2020

Remove locale from react-i18n now that it is available in i18n-utils #47446

Closed

lsl requested a review from yuliyan November 17, 2020 09:19

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 17, 2020

lsl requested a review from p-jackson November 17, 2020 09:19

p-jackson reviewed Nov 18, 2020

View reviewed changes

Update client/components/calypso-i18n-provider/index.tsx

cf74ca3

Co-authored-by: Philip Jackson <p-jackson@live.com>

lsl mentioned this pull request Nov 19, 2020

Revert calypso localizeUrl's move to i18n-utils package #47564

Merged

lsl added a commit that referenced this pull request Nov 19, 2020

Bring in changes from #47492

e80c69d

yuliyan approved these changes Nov 19, 2020

View reviewed changes

lsl mentioned this pull request Nov 20, 2020

Gutenboarding: i18n #37665

Closed

46 tasks

lsl closed this Nov 20, 2020

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 20, 2020

sirreal deleted the update/i18n-utils-unreverted branch January 14, 2025 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/i18n utils unreverted #47492

Update/i18n utils unreverted #47492

lsl commented Nov 17, 2020 •

edited

Loading

matticbot commented Nov 17, 2020

lsl Nov 17, 2020

yuliyan Nov 17, 2020

lsl Nov 18, 2020

p-jackson Nov 18, 2020

matticbot commented Nov 17, 2020 •

edited

Loading

p-jackson left a comment

p-jackson Nov 18, 2020

p-jackson Nov 18, 2020

yuliyan left a comment

yuliyan Nov 19, 2020

p-jackson Nov 19, 2020

lsl Nov 20, 2020

lsl commented Nov 20, 2020

Update/i18n utils unreverted #47492

Update/i18n utils unreverted #47492

Conversation

lsl commented Nov 17, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Nov 17, 2020 • edited Loading

p-jackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuliyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsl commented Nov 20, 2020

lsl commented Nov 17, 2020 •

edited

Loading

matticbot commented Nov 17, 2020 •

edited

Loading